-
Notifications
You must be signed in to change notification settings - Fork 121
[POS][Local Catalog] Always full sync, if site not in database #16099
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[POS][Local Catalog] Always full sync, if site not in database #16099
Conversation
|
|
…omob-1252-always-full-sync-if-site-not-in-database
jaclync
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We currently only persist one site at a time in the database.
Have we decided whether to support persisting catalog for one or multiple sites across platforms? I'd think supporting multiple sites would be nice, especially when we're only supporting small catalog size for storage space reason.
The lastSyncDate is stored in UserDefaults (perhaps we should store it on the Site entity instead?).
I thought the last full sync date is stored per site in the store settings? The state is persisted per site.
|
|
||
| @Test func shouldPerformFullSync_returns_true_when_no_previous_sync() { | ||
| // Given - no previous sync date stored | ||
| @Test func shouldPerformFullSync_site_not_in_database_with_no_sync_history() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
super nit:
| @Test func shouldPerformFullSync_site_not_in_database_with_no_sync_history() { | |
| @Test func shouldPerformFullSync_returns_true_when_site_is_not_in_database_with_no_sync_history() { |
| #expect(shouldSync == true) | ||
| } | ||
|
|
||
| @Test func shouldPerformFullSync_returns_true_when_no_previous_sync() throws { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
super nit:
| @Test func shouldPerformFullSync_returns_true_when_no_previous_sync() throws { | |
| @Test func shouldPerformFullSync_returns_true_when_site_is_in_database_with_no_previous_sync() throws { |
I was chatting to @malinajirka earlier – I don't think we have a decision, but it makes sense to support multiple sites. The cases are quite different for site credentials vs Jetpack login, and multi-site support for site credentials is a common user feature request... so we should build with it in mind. The only reason we support one site in the db right now is because I messed up the schema. Even though everything's tied to a Site record, the primary keys for Product and Variation (and other entities) still need to be unique across all sites. I'll fix that later by making them a compound key of
It is per site (store settings is User Defaults-backed... or at least in a plist somewhere.) I wondered about putting it on the Site entity so that we keep all the database information in the db. Might be a bad idea. But right now, user defaults could say "we synced If we kept the last sync date on the Site record, they'd always match up, because we delete the data for a site by deleting its record and letting that cascade to everything for the site. |
I agree, considering it's a lot effort I think it makes sense not to introduce unnecessary limitations. |
I implemented the last incremental sync date on the Site storage entity in #16108. If we're good with it, I'll create a task and update the last full sync date to the Site record as well. |
…omob-1252-always-full-sync-if-site-not-in-database


Part of: WOOMOB-1252
Merge after: #16098
Description
We currently only persist one site at a time in the database. The
lastSyncDateis stored in UserDefaults (perhaps we should store it on theSiteentity instead?).It's possible to have synced the site within the max sync time, but still not have it in the database. In that case, we still want to resync.
Steps to reproduce
RELEASE-NOTES.txtif necessary.